[Do Not Merge] chore: Shadow PR to external contribution #36231 #36361
Conversation
…f github.com:AnnaHariprasad5123/appsmith into chore/issue-36073/shadow-36231
NilanshBansal
requested review from
sharat87,
nidhi-nair and
sondermanish
as code owners
WalkthroughThe changes in this pull request focus on enhancing the logging practices across various plugins in the Appsmith application. The modifications involve replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin
participant Logger
User->>Plugin: Execute Method
Plugin->>Logger: log.debug("Executing method...")
Plugin->>Logger: log.error("An error occurred")
Logger-->>Plugin: Log output
Plugin-->>User: Return result
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
Backend
github-actions
bot
added
Bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (1)
137-139: Usinglog.errorfor the connection pool issue is spot-on! 🎯Logging an error message when there's a problem returning a connection to the pool is crucial for debugging. Great job using
log.errorfor this!One small suggestion though - let's remove the
printStackTracecall in production code. Stack traces can clutter the logs and should usually be avoided in prod. You can keep it for local debugging.To remove the stack trace, simply delete this line:
- e.printStackTrace();
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (27)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/Stopwatch.java (0 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java (19 hunks)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (3 hunks)
- app/server/appsmith-plugins/anthropicPlugin/src/main/java/com/external/plugins/AnthropicPlugin.java (4 hunks)
- app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/AppsmithAiPlugin.java (5 hunks)
- app/server/appsmith-plugins/arangoDBPlugin/src/main/java/com/external/plugins/ArangoDBPlugin.java (9 hunks)
- app/server/appsmith-plugins/awsLambdaPlugin/src/main/java/com/external/plugins/AwsLambdaPlugin.java (5 hunks)
- app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java (7 hunks)
- app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java (7 hunks)
- app/server/appsmith-plugins/elasticSearchPlugin/src/main/java/com/external/plugins/ElasticSearchPlugin.java (7 hunks)
- app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java (10 hunks)
- app/server/appsmith-plugins/googleAiPlugin/src/main/java/com/external/plugins/GoogleAiPlugin.java (3 hunks)
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java (6 hunks)
- app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java (2 hunks)
- app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java (17 hunks)
- app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java (10 hunks)
- app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java (17 hunks)
- app/server/appsmith-plugins/openAiPlugin/src/main/java/com/external/plugins/OpenAiPlugin.java (4 hunks)
- app/server/appsmith-plugins/oraclePlugin/src/main/java/com/external/plugins/OraclePlugin.java (10 hunks)
- app/server/appsmith-plugins/pom.xml (1 hunks)
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java (22 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (6 hunks)
- app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java (13 hunks)
- app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java (3 hunks)
- app/server/appsmith-plugins/saasPlugin/src/main/java/com/external/plugins/SaasPlugin.java (5 hunks)
- app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java (8 hunks)
- app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (13 hunks)
Files not reviewed due to no reviewable changes (1)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/helpers/Stopwatch.java
Files skipped from review due to trivial changes (22)
- app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/plugins/AmazonS3Plugin.java
- app/server/appsmith-plugins/anthropicPlugin/src/main/java/com/external/plugins/AnthropicPlugin.java
- app/server/appsmith-plugins/appsmithAiPlugin/src/main/java/com/external/plugins/AppsmithAiPlugin.java
- app/server/appsmith-plugins/arangoDBPlugin/src/main/java/com/external/plugins/ArangoDBPlugin.java
- app/server/appsmith-plugins/awsLambdaPlugin/src/main/java/com/external/plugins/AwsLambdaPlugin.java
- app/server/appsmith-plugins/databricksPlugin/src/main/java/com/external/plugins/DatabricksPlugin.java
- app/server/appsmith-plugins/dynamoPlugin/src/main/java/com/external/plugins/DynamoPlugin.java
- app/server/appsmith-plugins/elasticSearchPlugin/src/main/java/com/external/plugins/ElasticSearchPlugin.java
- app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
- app/server/appsmith-plugins/googleAiPlugin/src/main/java/com/external/plugins/GoogleAiPlugin.java
- app/server/appsmith-plugins/googleSheetsPlugin/src/main/java/com/external/plugins/GoogleSheetsPlugin.java
- app/server/appsmith-plugins/graphqlPlugin/src/main/java/com/external/plugins/GraphQLPlugin.java
- app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java
- app/server/appsmith-plugins/mssqlPlugin/src/main/java/com/external/plugins/MssqlPlugin.java
- app/server/appsmith-plugins/mysqlPlugin/src/main/java/com/external/plugins/MySqlPlugin.java
- app/server/appsmith-plugins/openAiPlugin/src/main/java/com/external/plugins/OpenAiPlugin.java
- app/server/appsmith-plugins/oraclePlugin/src/main/java/com/external/plugins/OraclePlugin.java
- app/server/appsmith-plugins/postgresPlugin/src/main/java/com/external/plugins/PostgresPlugin.java
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java
- app/server/appsmith-plugins/redshiftPlugin/src/main/java/com/external/plugins/RedshiftPlugin.java
- app/server/appsmith-plugins/restApiPlugin/src/main/java/com/external/plugins/RestApiPlugin.java
- app/server/appsmith-plugins/smtpPlugin/src/main/java/com/external/plugins/SmtpPlugin.java
Additional comments not posted (10)
app/server/appsmith-plugins/pom.xml (2)
56-57: Excellent work, class! 👏The dependency on
org.pf4j:pf4j-springwith a provided scope is declared correctly. This will enable the use of the PF4J plugin framework in our application. Keep up the good work!
58-67: Great job on improving our logging infrastructure! 🌟The addition of the
slf4j-apiandslf4j-simpledependencies with a provided scope is a step in the right direction. By leveraging SLF4J, we can achieve a more flexible and maintainable logging system. SLF4J provides a simple facade for various logging frameworks, allowing us to easily switch between different implementations without modifying our code.Moreover, the removal of the
slf4j-reload4jexclusion, as mentioned in the AI-generated summary, further streamlines our logging setup. This change will contribute to a cleaner and more efficient logging configuration.Keep up the excellent work in enhancing our application's logging capabilities! 📝👨🏫
app/server/appsmith-plugins/amazons3Plugin/src/main/java/com/external/utils/DatasourceUtils.java (1)
15-15: Great job on improving the logging practices! The changes look good to me.The introduction of the
@Slf4jannotation and the replacement ofSystem.out.printlnwith a structured logging statement are excellent improvements. These changes will enhance the maintainability and traceability of the codebase.A few additional suggestions to consider:
Ensure that the log level is set appropriately based on the criticality of the information being logged. Debug-level logs are suitable for development and troubleshooting purposes, but you might want to use a higher level (e.g., INFO or WARN) for important events in production.
Consider using parameterized logging instead of string concatenation. For example, instead of:
log.debug(Thread.currentThread().getName() + ": getS3ClientBuilder action called.");You can use:
log.debug("{}: getS3ClientBuilder action called.", Thread.currentThread().getName());This allows the logging framework to optimize the log message construction based on the configured log level.
Keep up the great work on improving the logging practices throughout the codebase!
Also applies to: 30-30, 106-106
app/server/appsmith-plugins/saasPlugin/src/main/java/com/external/plugins/SaasPlugin.java (2)
52-52: Great job utilizing the Lombok@Slf4jannotation! 👍This annotation automatically generates a logger field for the class, making it convenient to perform structured logging using the SLF4J API. It's a cleaner approach compared to manually creating logger instances.
90-90: Excellent work enhancing the logging mechanism! 🌟I noticed that you have replaced
System.out.printlnstatements withlog.debugstatements at key points in theexecutemethod. This is a great practice for capturing the execution flow and processes using structured logging.Additionally, you have switched from using
stopAndLogTimeInMillisWithSysOuttostopAndLogTimeInMillis, which aligns with the goal of relying on structured logging instead of standard output.These changes improve the maintainability and readability of the logging within the plugin's execution logic. Keep up the good work!
Also applies to: 139-143, 156-162
app/server/appsmith-plugins/snowflakePlugin/src/main/java/com/external/plugins/SnowflakePlugin.java (5)
74-74: Great job using a proper logging framework! 👍Using
log.debuginstead ofSystem.out.printlnis a best practice. The log message also provides helpful context about the thread and method being executed.
84-84: Excellent use of a logging framework! 🌟
log.debugis the way to go for logging. The message also helpfully indicates the thread and action. Keep up the good work!
Line range hint
109-116: Logging connection pool stats is a smart move! 🧠Fantastic work using
log.debugand including the query and pool stats in the log message. This will be super helpful for monitoring and troubleshooting. You're doing great!
Line range hint
128-132: Logging pool stats after the query is brilliant! 💡Using
log.debugand capturing the pool stats after query execution is an excellent practice. This will make monitoring and debugging much easier. Fantastic work, keep it up!
158-161: Logging at the start of key methods is a great debugging aid! 🔍Wonderful work adding
log.debugstatements at the beginning ofcreateConnectionClientand when creating the Snowflake connection client. Including the thread name and actions in the log messages is also very helpful for tracing and debugging.Your logging practices are top-notch, keep up the excellent work!
|
Closing this as the original PR #36231 has been merged |
Description
Fixes #36073
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10899078369
Commit: 383128d
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 17 Sep 2024 08:31:09 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
System.out.printlnwith structured logging usinglog.debugandlog.error.Bug Fixes
stopAndLogTimeInMillisWithSysOutfrom the Stopwatch class, improving logging consistency.Chores
These changes improve maintainability, performance, and debugging capabilities across the application.